Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix importlib-metadata on Python 3.8 + bump ale-py #2428

Merged
merged 2 commits into from
Sep 28, 2021
Merged

Fix importlib-metadata on Python 3.8 + bump ale-py #2428

merged 2 commits into from
Sep 28, 2021

Conversation

JesseFarebro
Copy link
Contributor

No description provided.

@JesseFarebro JesseFarebro changed the title Bump ale-py --> 0.7.1 Bump ale-py -> 0.7.1 Sep 28, 2021
@RedTachyon
Copy link
Contributor

RedTachyon commented Sep 28, 2021

I see you came across the same bug as I have in #2427. This might deserve an issue of its own, but now I understand even less how this happened just now.

In any case, we'd need to decide on which solution to use. From what I see, you wrote some code to emulate .attr behavior. I honestly prefer my lazy method of using importlib_metadata in Python 3.8 because simplicity.

@RedTachyon RedTachyon mentioned this pull request Sep 28, 2021
@JesseFarebro
Copy link
Contributor Author

Hey @RedTachyon, it happened just now as we just published a minor version of ale-py which changes some things.

RE: the solution: I'd prefer that as well, the issue being importlib-metadata likes throwing out its own deprecation warnings about SelectableGroups. It seems they won't fix this so I'd like to avoid more of these warnings if possible.

@RedTachyon
Copy link
Contributor

@JesseFarebro Did the new version change the registration placement of some environments? I'm trying to make sense of what happened, whether something just messed up the tests externally and it so happens that we made PR's now, or did we both just make PR's that actually expose the underlying bug, and happened to do them roughly at the same time?

In any case, to make sure I understand, you agree with using the "external" library importlib_metadata in 3.8 as well?

@JesseFarebro
Copy link
Contributor Author

The new release of ale-py uses the registration system. Once the new version of ale-py is installed it'll get loaded when you import gym. There was a bug with Python 3.8 and we both happened to catch this right now.

In any case, to make sure I understand, you agree with using the "external" library importlib_metadata in 3.8 as well?

Sorry, I should have been more clear, I would like this solution but importlib-metadata throws a warning when imported and they won't fix the root cause of the warning. I think manually parsing attr is probably the best solution for now to avoid having to use importlib-metadata. I want to get rid of that dependency ASAP, it has some other weird oddities, so I'd prefer to have it only for <3.8 not <=3.8.

Perhaps we should catch these warnings as they may get pretty annoying for users on 3.7 as this occurs right when you import Gym.

@RedTachyon
Copy link
Contributor

as this occurs right when you import Gym.

Is that really the case? I just made a clean conda installation of Python 3.7 with gym==0.20.0 and importlib-metadata-4.8.1 and get no warnings.

@JesseFarebro
Copy link
Contributor Author

JesseFarebro commented Sep 28, 2021

Gym 0.20 didn't include the plugin system. Install gym from git+https://github.com/openai/gym and ale-py==0.7.1 and it'll break on Python 3.8.

Edit: if you do what I said on Python 3.7 you'll see the warning as well.

@JesseFarebro JesseFarebro changed the title Bump ale-py -> 0.7.1 Fix importlib-metadata on Python 3.8 Sep 28, 2021
@JesseFarebro JesseFarebro changed the title Fix importlib-metadata on Python 3.8 Fix importlib-metadata on Python 3.8 + Bump ale-py Sep 28, 2021
@JesseFarebro JesseFarebro changed the title Fix importlib-metadata on Python 3.8 + Bump ale-py Fix importlib-metadata on Python 3.8 + bump ale-py Sep 28, 2021
@jkterry1 jkterry1 merged commit ca42b05 into openai:master Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants